@damiano-flex @marc-flex I am getting confused with the source types and the analysis types. Would one still be defining e.g. IsothermalSteadyChargeDCAnalysis when there are AC type sources?

@damiano-flex @marc-flex I am getting confused with the source types and the analysis types. Would one still be defining e.g. IsothermalSteadyChargeDCAnalysis when there are AC type sources?

Initially, I had a different Analysis type for AC analysis. It was virtually a clone. Marc suggested to keep old analysis types, and define AC at source level, and this idea seemed good to me as well. If you have an AC source, you'll get everything from the DC simulation + a graph for AC current.

An option might be that IsothermalSteadyChargeDCAnalysis drops the DC keyword or change its name, since: 1) the DC analysis will be always performed 2) the AC analysis will be performed after the DC analysis if an AC source has been defined.

@damiano-flex @marc-flex I am getting confused with the source types and the analysis types. Would one still be defining e.g. IsothermalSteadyChargeDCAnalysis when there are AC type sources?

Initially, I had a different Analysis type for AC analysis. It was virtually a clone. Marc suggested to keep old analysis types, and define AC at source level, and this idea seemed good to me as well. If you have an AC source, you'll get everything from the DC simulation + a graph for AC current.

An option might be that IsothermalSteadyChargeDCAnalysis drops the DC keyword or change its name, since: 1) the DC analysis will be always performed 2) the AC analysis will be performed after the DC analysis if an AC source has been defined.

I think that makes sense. Note that, for backwards compatibility, we'd keep IsothermalSteadyChargeDCAnalysis but issue a deprecation warning when using it. So something like

class IsothermalSteadyChargeAnalysis():
    # takes everything that's currently in IsothermalSteadyChargeDCAnalysis

class IsothermalSteadyChargeDCAnalysis(IsothermalSteadyChargeAnalysis):
    # add deprecation warning

Refer to the HeatSimulation.

@damiano-flex @momchil-flex I'm now having second thoughts about the current naming of sources and analysis types.

Right now we're saying that if we see an ACVoltageSource in a boundary condition we'll perform an SSAC simulation. But what if, in the future we can do non-linear simulations with AC sources? I guess we have two way forward:

1. We create small-signal equivalents of the SteadyChargeAnalysis and IsothermalSteadyChargeAnalysis as @damiano-flex suggested in the first place; or
2. We create SSACVoltageSource. I guess the only issue with this one is that it isn't very SPICE like

These type of charge simulations are different from FDTD simulations in the sense that a given "scene" with sources etc. can have different types of analysis depending on what you're looking into like transient or DC operating point, and so the data generated is inherently different per analysis type.

In any case:

We create small-signal equivalents of the SteadyChargeAnalysis and IsothermalSteadyChargeAnalysis as @damiano-flex suggested in the first place; or

would be then equivalent to this type of SS ac SPICE analysis directive that Damiano mentioned separately, but then the decision is whether the analysis type has the frequency sweep or the source. IMO it should be the analysis but use the source name to reference to it, and override any specified frequency in it.

.ac dec nd fstart fstop
.ac oct no fstart fstop
.ac lin np fstart fstop

Diff Coverage

Diff: origin/develop...HEAD, staged and unstaged changes

• tidy3d/init.py (100%)
• tidy3d/components/data/data_array.py (100%)
• tidy3d/components/frequencies.py (100%)
• tidy3d/components/spice/analysis/ac.py (95.5%): Missing lines 49
• tidy3d/components/spice/sources/ac.py (95.5%): Missing lines 67
• tidy3d/components/spice/sources/dc.py (100%)
• tidy3d/components/spice/sources/types.py (100%)
• tidy3d/components/spice/types.py (100%)
• tidy3d/components/tcad/data/sim_data.py (100%)
• tidy3d/components/tcad/simulation/heat_charge.py (90.9%): Missing lines 1988-1989,1991,1995

Summary

• Total: 111 lines
• Missing: 6 lines
• Coverage: 94%

tidy3d/components/spice/analysis/ac.py

  45 
  46     @pd.validator("freqs")
  47     def validate_freqs(cls, val):
  48         if len(val) == 0:
! 49             raise ValueError("'freqs' cannot be empty (size 0).")
  50         else:
  51             for freq in val:
  52                 if freq == td_inf:
  53                     raise ValueError("'freqs' cannot contain infinite frequencies.")

tidy3d/components/spice/sources/ac.py

  63 
  64     @pd.validator("amplitude")
  65     def validate_amplitude(cls, val):
  66         if val == td_inf:
! 67             raise ValueError(f"Signal amplitude must be finite. Current amplitude={val}.")
  68         return val

tidy3d/components/tcad/simulation/heat_charge.py

  1984 
  1985         return any(isinstance(source, HeatFromElectricSource) for source in self.sources)
  1986 
  1987     def _get_charge_type(self):
! 1988         if isinstance(self.analysis_spec, (SSACAnalysis, IsothermalSSACAnalysis)):
! 1989             return "ac"
  1990         else:
! 1991             return "dc"
  1992 
  1993     def _get_ssac_frequency_and_amplitude(self):
  1994         if not isinstance(self.analysis_spec, (SSACAnalysis, IsothermalSSACAnalysis)):
! 1995             raise SetupError(
  1996                 "Invalid analysis type for Small-Signal AC (SSAC). "
  1997                 "SSAC requires a 'SSACAnalysis' or 'IsothermalSSACAnalysis', "
  1998                 f"but received '{type(self.analysis_spec).__name__}' instead."
  1999             )

@damiano-flex @momchil-flex I'm now having second thoughts about the current naming of sources and analysis types.

Yeah I don't really know the answer but we must 100% think about it right now before things are released. It does seem to me that the current approach will be becoming more and more tedious/confusing as new regimes are added.

Maybe try to sketch what other analysis we want to support and how what the API might look like - even if it's for one or two years from now. Maybe let's take the discussion off this repo, too.

@greptile

The PR was modified completely due to a change of API structure. So, rewrite the PR body with the correct information referring to the updated code.

So I've had a brief chat with Damiano about circling back to the specific SSAC API.

Again from what I understand the main issue of not having Signal and a generic AC source is simply the issue that we have a voltage array on DC sources (which is a API design problem manifesting here) since the parameter sweeps make sense to be self contained to each Analysis and it just reference a source name for example.

That said, because I was on holiday, you guys have already discussed this and I don't want to hold back this PR any further. So if you want to merge this assuming it will be deprecated eventually when remove parameter sweeps from source definitions to have more AC source generality that's ok.

I don't agree this is a good approach since it's something we should avoid but if we're rushed to get this in then that's how it goes. We should avoid keeping introducing classes we know are going to be deprecated, and have to further support and just do it properly once. I understand the argument that the charge code-base is small enough to do this for now so it's just a matter of paying that price for the sake of introducing this functionality sooner.

Really in the future we should have discussed the API via a PRD and a TDD rather than jumping in to implement it and keeping discussing on circling back etc. and agree the user facing functionality early on.

We're breaking compatibility in 2.10 in RF already and we could do it for these specific things in Charge if we really want to fix them, or transform it internally to a new API structure (maybe part of a separate conversation).

It probably would be great to get @tylerflex 's thoughts on this.

@daquinteroflex if I understand things correctly, what we agreed on wouldn't require breaking compatibility. If we get to extend things to full transient anaysis, we will just keep separating the small-signal sources from the more general AC sources. The point is that the SSAC source is special and it's very constrained in the type of signal it supports. You can then think of it as a convenience component - it seemed like adding a more general component that can accommodate both SS and full transient in the future just adds verbosity to the SSAC setup, which will be the only thing we support for a while anyway.

Not sure I fully understand this though. Can you elaborate?

So if you want to merge this assuming it will be deprecated eventually when remove parameter sweeps from source definitions to have more AC source generality that's ok.

@daquinteroflex I don't think we'll deprecate the SSAC signal BC. It's very specific so I think it makes sense to keep it.

So just for the record I think the solution as you describe will "work inline to what we have been doing before". So it's not different from our previous charge API approaches, and should provide the desired functionality for now. I just think we're keeping on building on previous implicit behaviour issues that may be confusing (already maybe they are) as the functionality increases, and we're not taking this as an opportunity to address known issues of the electrical setup and workflow user clarity that can become more confusing. It is fine by me if we want to merge it for the sake of getting this in, but I describe below why this could introduce technical debut and further lack of workflow clarity for the users even if it introduces more verbosity in the backend.

This is what I was proposing mainly through having a more explicit Analysis separation and configuration, and introduce nodal treatment to help those analysis specify what should run. I illustrate what I mean below on the line of your request Momchil.

So if you want to merge this assuming it will be deprecated eventually when remove parameter sweeps from source definitions to have more AC source generality that's ok.

Currently we have parameter sweeps in the source definitions, which introduces multivariable input complexity to manage also depending on the type of analysis performed. For example, DCVoltageSource(voltages=[....]) will have different treatments that might be unclear to the user according to the different analysis performed. Say someone uses this SSACVoltageSource(voltages=[.....], freqs=[.....]) , then they're actually performing an "SSAC analysis" for each DC voltage point right? From a user perspective, we're adding ambiguity what exactly is running as a "source" is not and should not be a parameter sweep in my opinion in this context since it can confuse what exactly it does or occurs in the backend. Introducing the verbosity is not necessarily a bad thing, it can clarify to users exactly what they are defining even if it is harder for us to deal with it possibly?

We can make this more explicit by simply specifying source names, like nodes, and then have parameter sweeps only be defined at the analysis level. So the case above:

SSACVoltageSource(voltages=[.....], freqs=[.....]) ----->  becomes something like (in the current architecture

[
HeatChargeSimulation(..., VoltageBC(DCVoltageSource(voltage=1, name="V0")), SSAnalysis(freqs=[...], name="V0")), 
HeatChargeSimulation(....),
HeatChargeSimulation(....)
]

Like this approach means it is explicit they know an SSAC is being run as a "single heat-charge simulation" configuration independent of the other DC voltage sweeps they are doing so it explains what's actually going on. We could do a similar thing for DCAnalysis where the parameter sweep is specified at the analysis class because that way it's clearer what's going on and how their configuration changes their final results. If it is not the case, then we probably want to make this very explicit. When we did the first charge API, management of what is actually run under the hood in the backend was not very explicit so trying to avoid the combinations of settings get unbounded and users don't know what's going on as the simulations become more complex.

it seemed like adding a more general component that can accommodate both SS and full transient in the future just adds verbosity to the SSAC setup

So the approach above simply separates "analysis configuration" to the analysis class, rather than mixing it in the source class possibly.

You can then think of it as a convenience component - it seemed like adding a more general component that can accommodate both SS and full transient in the future just adds verbosity to the SSAC setup, which will be the only thing we support for a while anyway.

I don't think we'll deprecate the SSAC signal BC. It's very specific so I think it makes sense to keep it.

So this all kind of depends if we want to keep embedding SSAC analysis-specific stuff into the simulation/scene definition rather than simply treat the analysis to be defined based on the "physical/electrical node scene" of the simulation.

This will work, so if we just want to keep doing this it's ok. I just feel we're keeping on building implicit treatment rather than explict treatment of how the simulation is being processes. Say how this is going to work if people want to eventually reuse "physical scenes" or even as they begin adding sources that are incompatible with a given analysis just feels we might introduce a bunch of edge cases and not catch the users intention explicitly possibly. Again, it's what we've been doing already and "it works", but feel one day we need to deal with these issues of explicit behavior.

Note that this is not what this PR is introducing: SSACVoltageSource(voltages=[.....], freqs=[.....])

There are no freqs in the source itself. Instead the ssac frequencies go in the analysis spec. This is because eventually you could have multiple SSAC sources, and the analysis should to be done at the same frequency for all of them to get the admittance matrix at that frequency.

Ahh, sorry, the API has been changing a lot and the diffs got me mixed up

Note that this is not what this PR is introducing: SSACVoltageSource(voltages=[.....], freqs=[.....])
There are no freqs in the source itself. Instead the ssac frequencies go in the analysis spec. This is because eventually you could have multiple SSAC sources, and the analysis should to be done at the same frequency for all of them to get the admittance matrix at that frequency.

Yeah this works well